Python: Allow models-as-data sanitizers #21234
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables models-as-data (MaD) sanitizers for Python security queries and converts the Flask path-injection sanitizer from CodeQL to a MaD model. The key change is fixing the barrierNode implementation to use asSink() instead of asSource(), which correctly identifies where data flows into sanitizing functions.
Changes:
- Fixed
barrierNodeimplementation in ApiGraphModels.qll to useasSink()for proper barrier node identification - Converted Flask
send_from_directoryfilename sanitizer from CodeQL class to MaD YAML model - Added
SanitizerFromModelclasses across 8 security query customization files to support MaD-defined sanitizers - Added
CredentialSanitizerclass in HardcodedCredentials.ql to support credential-specific MaD sanitizers
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll | Fixed barrierNode to use asSink() instead of asSource() for correct identification of nodes where data flows into barriers |
| python/ql/lib/semmle/python/frameworks/Flask.qll | Removed FlaskSendFromDirectoryCallFilenameSanitizer class (converted to MaD) |
| python/ql/lib/semmle/python/frameworks/Flask.model.yml | Added MaD barrier model for Flask send_from_directory filename argument |
| python/ql/lib/semmle/python/security/dataflow/UrlRedirectCustomizations.qll | Clarified comment that SanitizerFromModel sanitizes all flow states |
| python/ql/lib/semmle/python/security/dataflow/UnsafeDeserializationCustomizations.qll | Added SanitizerFromModel class for unsafe-deserialization kind |
| python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll | Added SanitizerFromModel class for sql-injection kind |
| python/ql/lib/semmle/python/security/dataflow/ReflectedXSSCustomizations.qll | Added SanitizerFromModel class for html-injection and js-injection kinds |
| python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll | Added SanitizerFromModel class for path-injection kind |
| python/ql/lib/semmle/python/security/dataflow/LogInjectionCustomizations.qll | Added SanitizerFromModel class for log-injection kind |
| python/ql/lib/semmle/python/security/dataflow/CommandInjectionCustomizations.qll | Added SanitizerFromModel class for command-injection kind |
| python/ql/lib/semmle/python/security/dataflow/CodeInjectionCustomizations.qll | Added SanitizerFromModel class for code-injection kind |
| python/ql/src/Security/CWE-798/HardcodedCredentials.ql | Added CredentialSanitizer class to support MaD barriers for credential sinks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
owen-mc
force-pushed
the
python/convert-sanitizers-to-mad
branch
from
fbf9d1d to
7232568
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| query: Security/CWE-089/SqlInjection.ql | ||
| postprocess: utils/test/PrettyPrintModels.ql |
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
owen-mc
force-pushed
the
python/convert-sanitizers-to-mad
branch
from
7232568 to
a3885cd
Compare
|
@aschackmull pointed out that |
tausbn
left a comment
tausbn
left a comment
There was a problem hiding this comment.
One comment, otherwise this looks good to me!
| this = any(FileSystemAccess e).getAPathArgument() and | ||
| not this = any(FileSystemAccess e).getASafePathArgument() and |
There was a problem hiding this comment.
I wonder if it would make sense to also add getAnUnsafePathArgument to FileSystemAccess, with an implementation consisting of (essentially) these two lines.
That way, the next time we write a query that depends on FileSystemAccess, it's (hopefully) harder to accidentally forget to filter out the safe path arguments.
There was a problem hiding this comment.
Good point. That makes me think, maybe instead of getASafePathArgument with default none() would could have getAnUnsafePathArgument with default getAPathArgument() and use that in the path injection query.
owen-mc
changed the title
2 participants
Note the commit "Fix implementation of barrierNode for barrier models" was needed to make the commit "Convert Flask path-injection sanitizer to MaD" work. I am not certain that it is the correct approach, so please review this carefully.
Also carefully review the MaD that I wrote, as I have no experience with MaD for dynamic languages.
There are two other sanitizers which seemed convertible, but only sanitize certain flow-states. We could in future extend the url-redirection
kindstring to make it possible to specify this. This would have a much greater benefit if the use of flow states for URL redirection is first implemented for all languages.